Skip to content

Conversation

NickHardeman
Copy link
Contributor

@NickHardeman NickHardeman commented Feb 15, 2025

Proposing this update to ofxSvg with the following implementation based on my ofx::svg::Parser addon (https://github.com/NickHardeman/ofxSvgParser).
This rewrite adds the following functionality:

  • Loading of elements with search functionality

  • Remove libtinysvg and libxml2 dependencies. The parsing is happening in the ofxSvg class ( it took a while getting through the spec on paths :0 and parsing/writing CSS manually ;) )

  • Add elements to the document

  • Save the document after loading or adding / removing elements.

  • Works with the current ofxSvg example.

Grab a group by passing in the name of the group.
auto logoGroup = svg.get("logo");

Grab nested groups by passing in the hierarchy to the group separated by colons.
auto cloudGroup = svg.get("sky:clouds");

Get all of the elements of a certain type by calling getElementsForType
auto sprinklePaths = svg.getElementsForType("Donut:Sprinkles");

Currently supports the following types:
Group, Rectangle, Image (linked and embedded), Ellipse, Circle, Path, Polygon and Line

Limited Parsing Support:
Text

We have been using some version of this for parsing svg in our projects for the last several years.

TODO:

  • Update scripts to remove libsvgtiny and libxml2
  • Add example for creating a svg document, adding items to it and saving.
  • Implement same constructors as previous ofxSvg ( copy and with default file path string ).
  • Complete in-line documentation.
  • Fix failed builds on certain platforms.
  • Discuss use of ofx::svg for subsequent classes, ofx::svg::Group vs. ofxSvgGroup. (ofxAssimp - uniformity on addon name, include name #8354)
  • Add image embedding saving
  • Image embed parsing.
  • Basic text parsing
  • Set text on text span with wrapping
  • Add text to document for saving
  • Use custom optional instead of std::optional to legacy and platform compatibility (fixes for windows oF 0.12.0 NickHardeman/ofxSvgParser#2)

@NickHardeman NickHardeman self-assigned this Feb 15, 2025
@NickHardeman
Copy link
Contributor Author

This issue on windows might make the argument for ofxSvgEllipse instead of ofx::svg::Ellipse

../../../addons/ofxSvg/src/ofxSvg.cpp:1988:17: error: reference to 'Ellipse' is ambiguous
   1988 | std::shared_ptr<Ellipse> ofxSvg::addEllipse( const glm::vec3& apos, float aradiusX, float aradiusY ) {
        |                 ^
  D:/a/_temp/msys64/clang64/include/wingdi.h:3009:28: note: candidate found by name lookup is 'Ellipse'
   3009 |   WINGDIAPI WINBOOL WINAPI Ellipse(HDC hdc,int left,int top,int right,int bottom);

@dimitre
Copy link
Member

dimitre commented Feb 15, 2025

Fantastic @NickHardeman let me know when it is ready to test and I can test with a lot of SVGs from previous projects here

@NickHardeman
Copy link
Contributor Author

Fantastic @NickHardeman let me know when it is ready to test and I can test with a lot of SVGs from previous projects here

Great, will do. I can also send a test app that loads an svg from a folder, saves it and reloads the saved one. Arrow keys to change index and load again in the directory.

@NickHardeman
Copy link
Contributor Author

@dimitre I think it's ready for some testing now that it's passed the checks. Attached is a quick app for testing loading, saving and loading the saved svgs. The svgs in the folder are from https://www.svgviewer.dev and are just for testing complex paths.
SvgTester.zip

@dimitre
Copy link
Member

dimitre commented Feb 21, 2025

Great I've just seen now and tested some things. I'm testing with a very crude set of svgs and will report some things.
sharing the files here if you want to check
svgs.zip

@dimitre
Copy link
Member

dimitre commented Feb 21, 2025

Most of the things work great for reading and rewriting.
I've noticed in art made with lines only, contours are thicker than the macos quickLook preview
and open outlines appear as closed.
Novapadrao6
Anitta2

@NickHardeman
Copy link
Contributor Author

@dimitre thank you for trying it out. I have fixed some of the fill issues that you mentioned.

@NickHardeman
Copy link
Contributor Author

I think the line width is due to the ofScale I am applying in the example to get the entire svg to fit within the window. When I remove the scaling, here is a comparison between OF and osx quicklook preview. The preview is on top and the OF app is in the bg.

image

@NickHardeman
Copy link
Contributor Author

Lines are scaled with attenuation by the very hidden function

auto rend = std::dynamic_pointer_cast<ofGLProgrammableRenderer>( ofGetCurrentRenderer() );
rend->enableLineSizeAttenuation();

Screenshot 2025-02-21 at 10 27 53 PM

Default screen space lines:
Screenshot 2025-02-21 at 10 28 15 PM

@dimitre
Copy link
Member

dimitre commented Jul 28, 2025

Hey Nick finally I've found this one again. I'm looking forward to see it merged :)
I'll cherry pick to the branch I'm using now.

@dimitre
Copy link
Member

dimitre commented Aug 8, 2025

it would be great to have this one merged. if you prefer it can have an alternative name, like ofxAssimpModelLoader / ofxAssimp

@NickHardeman
Copy link
Contributor Author

@dimitre I agree, been doing a lot of work on the addon, implementing some text parsing and cleaning it up a bit. Text parsing is exhausting!
Was hoping to just keep ofxSvg and it would replace the existing addon to avoid the continued maintenance of libsvgtiny and a new addon. Though the path parsing should function the same, if it doesn't there wouldn't be access to the old addon it it was replaced...
I implemented the previous functions so it should be compatible with the previous addon. As with all new addons it will need some more thorough testing. Thank you for the tests you have already provided, I have been using them while developing :)

@dimitre
Copy link
Member

dimitre commented Aug 14, 2025

Awesome!

@dimitre
Copy link
Member

dimitre commented Aug 15, 2025

I'm now testing and found some issues with this files here
Archive.zip

@NickHardeman
Copy link
Contributor Author

Due to the nature of ofPath, overlapping sub paths result in holes.

Screenshot 2025-08-22 at 5 10 56 PM Screenshot 2025-08-22 at 5 11 07 PM

@NickHardeman NickHardeman marked this pull request as ready for review September 4, 2025 14:11
@NickHardeman NickHardeman requested a review from dimitre September 4, 2025 14:11
@NickHardeman NickHardeman requested a review from ofTheo September 4, 2025 17:06
@ofTheo
Copy link
Member

ofTheo commented Sep 4, 2025

Due to the nature of ofPath, overlapping sub paths result in holes.

Screenshot 2025-08-22 at 5 10 56 PM Screenshot 2025-08-22 at 5 11 07 PM

Can this be fixed with one of the winding orders?

Copy link
Member

@ofTheo ofTheo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll let @dimitre do a final test - but this all looks good to me!
Nice to have ofNode support too for local / global transformations

@NickHardeman
Copy link
Contributor Author

NickHardeman commented Sep 4, 2025

Due to the nature of ofPath, overlapping sub paths result in holes.
Screenshot 2025-08-22 at 5 10 56 PM Screenshot 2025-08-22 at 5 11 07 PM

Can this be fixed with one of the winding orders?

Not sure, but the tricky bit is that the 'O' has an internal subpath and renders correctly. So determining when it should apply different winding order would need to be implemented. Maybe check for an intersection and handle it differently?

@NickHardeman
Copy link
Contributor Author

Another item to consider is the removal of the svg tiny libs. Not sure about the best approach...

@NickHardeman
Copy link
Contributor Author

NickHardeman commented Sep 6, 2025

Screenshot 2025-09-06 at 2 41 52 PM

@ofTheo was right; I just tried changing the winding order and it looks correct with OF_POLY_WINDING_POSITIVE. However, calling getOutlines() no longer returns a vector of ofPolylines since there is no stroke width on the path.
And it breaks other types of paths. So we can maybe just add it an example of how to call
spath->getPath().setPolyWindingMode(OF_POLY_WINDING_POSITIVE);

void ofPath::tessellate(){
    generatePolylinesFromCommands();
    if(!bNeedsTessellation || polylines.empty() || std::all_of(polylines.begin(), polylines.end(), [](const ofPolyline & p) {return p.getVertices().empty();})) return;
    if(bFill){
        tessellator.tessellateToMesh( polylines, windingMode, cachedTessellation);
    }
    if(hasOutline() && windingMode!=OF_POLY_WINDING_ODD){
        tessellator.tessellateToPolylines( polylines, windingMode, tessellatedContour);
    }
    bNeedsTessellation = false;
}


//----------------------------------------------------------
const vector<ofPolyline> & ofPath::getOutline() const{
    if(windingMode!=OF_POLY_WINDING_ODD){
        const_cast<ofPath*>(this)->tessellate();
        return tessellatedContour;
    }else{
        const_cast<ofPath*>(this)->generatePolylinesFromCommands();
        return polylines;
    }
}

Wondering if we could duplicate this functionality
We may want to add another boolean to track if the outlines need to be tessellated.
tessellator.tessellateToPolylines( polylines, windingMode, tessellatedContour);
into

const vector<ofPolyline> & ofPath::getOutline() const{
    if(windingMode!=OF_POLY_WINDING_ODD){
        ////const_cast<ofPath*>(this)->tessellate(); <--- remove this 
        // add below 
        generatePolylinesFromCommands();
        if(!bNeedsContourTessellation || polylines.empty() || std::all_of(polylines.begin(), polylines.end(), [](const ofPolyline & p) {return p.getVertices().empty();})) return;
        tessellator.tessellateToPolylines( polylines, windingMode, tessellatedContour); 
        bNeedsContourTessellation = false;
        return tessellatedContour;
    }else{
        const_cast<ofPath*>(this)->generatePolylinesFromCommands();
        return polylines;
    }
}

…f OF_POLY_WINDING_NONZERO with a function to set it differently.
@NickHardeman
Copy link
Contributor Author

The ofPath::getOutline() seems like a separate issue and I think can be addressed outside of this thread.
I added functionality to use a default winding mode if all of the sub paths of an ofPath are closed. The default is OF_POLY_WINDING_NONZERO and can be set with
ofxSvg::setDefaultClosedPathWindingMode(ofPolyWindingMode)
It does not apply if a sub path is open because it seems to force close the polylines when rendering.

@ofTheo
Copy link
Member

ofTheo commented Sep 7, 2025

Cool - I am good to merge this!
@dimitre ?

@dimitre
Copy link
Member

dimitre commented Sep 7, 2025

Super! I'll be able to test next week and report back if anything.
but feel free to merge and fix later if needed.

@ofTheo ofTheo merged commit 2cc4b71 into openframeworks:master Sep 8, 2025
17 checks passed
@dimitre
Copy link
Member

dimitre commented Sep 8, 2025

I'm now testing ofxSvg. I had some issues with this SVGs and I'll be testing more soon
Archive.zip

@NickHardeman
Copy link
Contributor Author

hmm, I just tested the Archive.zip and they seem to render as they should. Can you talk more about the issues you are seeing?
Screenshot 2025-09-08 at 12 53 00 PM

@dimitre
Copy link
Member

dimitre commented Sep 8, 2025

heyyy sorry ignore the last one. they are all good. I merged changes somewhere else and tested in another OF copy.
I'll be testing lots of different SVGs soon and report back

@dimitre
Copy link
Member

dimitre commented Sep 8, 2025

I have issue with this one (hanging the software) here
pa2.svg.zip

@NickHardeman
Copy link
Contributor Author

NickHardeman commented Sep 8, 2025

Screenshot 2025-09-08 at 3 53 09 PM

I wasn't able to reproduce the issue. Maybe a build clean? I have been using Xcode to test.

@dimitre
Copy link
Member

dimitre commented Sep 11, 2025

Ok more info now. it works OK if I comment out
settings.setGLVersion(3,2);

It loads OK but hangs trying to draw element number 114 in this SVG.
using settings.setGLVersion(3,2);

I'll update with more info later

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants